Skip to content

fix(sqlite-native): gate native read cache behind env var#4630

Open
NathanFlurry wants to merge 1 commit intomainfrom
04-12-fix_sqlite-native_gate_native_read_cache_behind_env_var
Open

fix(sqlite-native): gate native read cache behind env var#4630
NathanFlurry wants to merge 1 commit intomainfrom
04-12-fix_sqlite-native_gate_native_read_cache_behind_env_var

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Copy Markdown
Member Author

NathanFlurry commented Apr 13, 2026

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown

claude bot commented Apr 13, 2026

PR #4630 Review: fix(sqlite-native) — Gate native read cache behind env var

Overview

This PR makes the native SQLite VFS read cache opt-in via the RIVETKIT_SQLITE_NATIVE_READ_CACHE env var (disabled by default) to match WASM VFS behavior. The change converts KvFileState.read_cache from HashMap<Vec<u8>, Vec<u8>> to Option<HashMap<Vec<u8>, Vec<u8>>> and guards all cache accesses with if let Some(read_cache) checks.


Correctness

Looks correct overall. All cache paths (read, write-through, truncate-invalidation, batch-flush) are consistently guarded.

One minor inefficiency in kv_io_file_control (the batch-flush path):

// Current code — allocates a Vec even when cache is disabled, then immediately drops it
let flushed: Vec<_> = std::mem::take(&mut state.dirty_buffer).into_iter().collect();
if let Some(read_cache) = state.read_cache.as_mut() {
    for (chunk_index, data) in flushed { ... }
}

When read_cache is None, the std::mem::take + into_iter().collect() allocates a Vec that is immediately dropped. The dirty_buffer is a BTreeMap, so std::mem::take already gives an owned, iterable value — the intermediate Vec is unnecessary. A tighter version:

let flushed = std::mem::take(&mut state.dirty_buffer);
if let Some(read_cache) = state.read_cache.as_mut() {
    for (chunk_index, data) in flushed {
        let key = kv::get_chunk_key(file.file_tag, chunk_index);
        read_cache.insert(key.to_vec(), data);
    }
}

This avoids the allocation when the cache is disabled and removes the redundant collect.


Design

  • OnceLock for the env var is the right pattern — reads once at first call and caches the result for the lifetime of the process. Correct behavior.
  • Storing read_cache_enabled in VfsContext means the setting is fixed at KvVfs::new() time and propagated to each KvFileState. This is intentional and correct; changing the env var after initialization has no effect, which is the expected behavior for a startup flag.
  • Env var parsing ("1" | "true" | "yes" | "on") is user-friendly. No issues there.

Documentation

The new RIVETKIT_SQLITE_NATIVE_READ_CACHE env var is not documented anywhere. Per CLAUDE.md, config/behavior changes should be reflected in docs. Worth adding a note to website/src/content/docs/self-hosting/configuration.mdx or a dedicated SQLite/native VFS page so operators know this knob exists.


Code Style

  • Comments added to truncate and batch-flush paths are clear and follow the project's sentence-style convention. Good.
  • No formatting issues observed.

Summary

Correctness All cache paths consistently guarded
Minor inefficiency Needless Vec allocation in batch-flush when cache is disabled (see above)
Documentation New env var not documented
Style Follows project conventions

The core change is solid. The Vec allocation nit and missing docs are the only things worth addressing before merge.

@NathanFlurry NathanFlurry changed the base branch from 04-12-fix_sqlite-native_replace_stderr_sql_trace_logging to graphite-base/4630 April 13, 2026 05:38
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-native_gate_native_read_cache_behind_env_var branch from a60a42d to 9d72e25 Compare April 13, 2026 05:38
@NathanFlurry NathanFlurry changed the base branch from graphite-base/4630 to 04-12-fix_pegboard_isolate_runner_config_dc_lookup_failures April 13, 2026 05:38
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_pegboard_isolate_runner_config_dc_lookup_failures branch from 0f0530c to 7702123 Compare April 13, 2026 05:50
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-native_gate_native_read_cache_behind_env_var branch 2 times, most recently from 2bb44ed to bf17632 Compare April 13, 2026 07:03
@MasterPtato MasterPtato force-pushed the 04-12-fix_pegboard_isolate_runner_config_dc_lookup_failures branch from 2759537 to 47e66e5 Compare April 13, 2026 19:35
@MasterPtato MasterPtato changed the base branch from 04-12-fix_pegboard_isolate_runner_config_dc_lookup_failures to graphite-base/4630 April 13, 2026 20:57
@NathanFlurry NathanFlurry force-pushed the 04-12-fix_sqlite-native_gate_native_read_cache_behind_env_var branch from bf17632 to 93e8897 Compare April 13, 2026 21:07
@NathanFlurry NathanFlurry changed the base branch from graphite-base/4630 to main April 13, 2026 21:07
@NathanFlurry NathanFlurry marked this pull request as ready for review April 14, 2026 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant